Feat: Seed remote feature flag controller with default flags - #9747
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
2b81ab9 to
8ce7cc1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a77816d. Configure here.
|
|
||
| // Rebuild the processed remote layer from last session's effective flags by | ||
| // stripping local overrides. | ||
| const processedRemoteFeatureFlags = { |
There was a problem hiding this comment.
Nit: This step is unnecessary, and doesn't achieve the stated goals
There is no reason to attempt to strip out the impact of previous overrides here. And even if we wanted to, this would not achieve that goal (because we don't know what the overrides were when this state was generated).
This code is assuming the overrides are unchanged. But if that was true, why remove them only to re-apply them immediately afterwards? The only reason I can foresee for re-applying overrides is to accommodate a change in overrides, which violates the assumption made here.
There was a problem hiding this comment.
If we really want to accommodate making the initial state of remoteFeatureFlags correct even when overrides change, we need to recalculate the feature flags from the raw values. Not from the persisted processed values.
If this is not a priority, we can simply delete this step instead.
There was a problem hiding this comment.
I see this was a pre-existing bug though, just moved by this PR.
| remoteFeatureFlags: { ...this.#processedRemoteFeatureFlags }, | ||
| remoteFeatureFlags: this.#getEffectiveFeatureFlags( | ||
| this.#processedRemoteFeatureFlags, | ||
| {}, |
There was a problem hiding this comment.
Nit: This is another pre-existing bug: this.#processedRemoteFeatureFlags may already contain overrides, because the attempt to strip them out int he constructor is ineffective. We must construct the feature flags from the raw values to remove overrides.
There was a problem hiding this comment.
+1 - Since @davidmurdoch mentioned that he's working on updating these states, I'll reach out to coordinate on this. I contemplated on doing a refactor but is out of scope for this PR.
| ...this.state, | ||
| localOverrides: newLocalOverrides, | ||
| remoteFeatureFlags, | ||
| remoteFeatureFlags: this.#getEffectiveFeatureFlags( |
There was a problem hiding this comment.
Nit: Another pre-existing bug, where we're attempting to remove overrides incorrectly
There was a problem hiding this comment.
+1 - Since @davidmurdoch mentioned that he's working on updating these states, I'll reach out to coordinate on this. I contemplated on doing a refactor but is out of scope for this PR.
| remoteFeatureFlags: this.#getEffectiveFeatureFlags( | ||
| redactedProcessedFlags, | ||
| ), | ||
| rawRemoteFeatureFlags: redactMetaMetricsIds(remoteFeatureFlags), |
There was a problem hiding this comment.
Ah, this explains the other bugs.
Redacting IDs here makes it impossible to use rawRemoteFeatureFlags for any useful work. This is a mistake, and is probably what motivated the flawed attempts at removing overrides. We must redact IDs later at the point of collection, not here. We need complete raw flags in order to effectively work with overrides.
6 participants
Explanation
This is part of an effort to keep the RemoteFeatureFlagController as the source of truth for feature flags. As part of that effort, we've added a new optional constructor arg named
defaultFeatureFlags, which will be provided by the platform apps. Under the hood, the controller will account for these flags when processing the effective flags that the consumers will use. The order of priority for the flags are - default flags > remote flags > override flags.References
Checklist
Note
Low Risk
Additive optional API with centralized merge logic and broad test coverage; behavior change is limited to consumers that pass defaults or rely on override removal without a remote value.
Overview
Adds an optional
defaultFeatureFlagsconstructor option so platform apps can seedRemoteFeatureFlagControllerwith client-side defaults that are not persisted. Effective flags exposed inremoteFeatureFlagsare now merged through#getEffectiveFeatureFlagswith precedence defaults → processed remote → local overrides.Remote fetch, override set/remove/clear, and constructor hydration all use that helper so defaults remain for flags the server omits, remote values win on conflicts, and removing an override falls back to remote or default when no remote value exists. The wallet initializer forwards
instanceOptions.remoteFeatureFlagController.defaultFeatureFlagsinto the controller.Reviewed by Cursor Bugbot for commit 615b161. Bugbot is set up for automated code reviews on this repo. Configure here.